-
Notifications
You must be signed in to change notification settings - Fork 534
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shell script rewrite to comply with POSIX and best practices #1908
base: master
Are you sure you want to change the base?
Conversation
prefCleaner works fine for me, however updater doesn't seem to be able to append overrides. |
Seems to be just a broken check? This fixes it and updater seems to work fine now. |
Error in both scripts - you were checking for the literal file named // from
SCRIPT_FILE=$(preadlink "$0") && [ -f SCRIPT_FILE ] || exit 1
// to
SCRIPT_FILE=$(preadlink "$0") && [ -f $SCRIPT_FILE ] || exit 1 and for your missing /bin/sh question - no it doesn't seem to work. |
@9ao9ai9ar Thoughts on using |
The POSIX way to do it is to overwrite the shebang with the path of
|
Hey @9ao9ai9ar @MagicalDrizzle I've noticed that progress on this PR is slowing down, so I'm willing to create a checklist of things to ensure that the script is working properly, and then make a GitHub workflow to test the script on different platforms. Here are some things to be checked:
This will tick the GitHub available runners off the list of things that testers have to check manually. Manual tester can then copy the workflow to test locally. What do you think? An alternative would be splitting this PR into smaller PRs. |
@9ao9ai9ar prefsCleaner works, updater only partly - works only if there's already an user.js file. It won't download a new user.js if doesn't exist, then will try to backup the nonexistent user.js file which fails.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like your improvements to the pipeline, I find it way more readable than my version. Are you planning on getting this merged now? I'd be interested in contributing a few more changes after this gets merged, such as an extra step for checking for normal execution
Yes, it's ready for merging.
You might want to check out ShellSpec. Writing the tests in the workflow file is cumbersome, limiting and a form of vendor lock-in. For example, I can't just run the tests in my VM without setting it up as a self-hosted runner on GitHub or relying on tools such as act, which you mentioned has some differing behaviors from the hosted runners. Ideally, the only workflow step you need after installing and setting up the shells is to call ShellSpec or some command to invoke the test scripts. |
The rewrite is focused on the following five areas of interest: 1. Portability. The scripts have been tested to work in recent versions of the following operating systems and shells: macOS, Linux (Fedora, Debian, Ubuntu, openSUSE, Arch, Alpine, NixOS), BSD (FreeBSD, OpenBSD, NetBSD, DragonFly), SunOS (Solaris, OpenIndiana), Haiku; bash, dash, ash, ksh, oksh, zsh, XPG4 sh, pdksh, mksh, yash, posh, gwsh, bosh, osh. 2. Robustness. Employ secure shell scripting techniques, incorporate battle-tested open source code, clear all ShellCheck warnings, and fail early. 3. Composability. Put (almost) everything inside functions and make the scripts dot source friendly. 4. Consistency. Use tput to abstract away terminal color codes, write templated diagnostic messages and follow conventions in the use of exit status and redirections. 5. Readability. Comment extensively, assign descriptive names to variables and functions, and use here-documents to ease reading and writing multi-line messages. Known behavioral changes: 1. There are changes to the way some options are parsed and acted on. For example, when both the -l and -p options are specified, -l will be ignored; in the old behavior, the last specified option would take effect. Also, an old quirk where passing the argument 'list' to -p was equivalent to specifying the -l option has been fixed. 2. The -h, -l and -p options of updater.sh have been added to prefsCleaner.sh as well. 3. All temporary files are now created using mktemp and no longer actively deleted, so users won't find them in the working directory anymore in the case of error. 4. The old prefs.js cleaning logic, which relied on non-POSIX features, is not preserved in the rewrite. Resolves arkenfox#1855 Resolves arkenfox#1446 Fixes arkenfox#1810
Thanks for the pointers! |
Resolves #1855
Resolves #1446
Fixes #1810
This is not the first POSIX solution to have ever been requested or presented, though certainly the most comprehensive one yet. Regrettably, the robustness goal hasn't truly been realized, so it would need to be addressed in a separate pull request. I'd also like to point out that alternatives like the one in #1696 exist and are worth considering.
Target Platforms for Modern Desktop Versions of Firefox
Based on the supported build hosts and targets of Firefox.
Tier-1
Windows(WSL, Cygwin, MSYS2, etc.) - Not targeted by the shell scripts.Tier-2
psmisc
orlsof
package for prefsCleaner.sh to work.Tier-3
Non-Tiered
POSIX Compliant Shells
/bin/sh
) - Automated testing only./bin/sh
and user shell)/bin/sh
)/bin/sh
and user shell)/bin/sh
and user shell)/bin/sh
and user shell)emulate sh
before sourcing.sh
)command return
and\return
can't suppress function/alias lookup and certain special properties in OSH, becausereturn
is a keyword oils-for-unix/oils#2228.hush- Not targeted for the time being due to missing crucial features:command -p
and thenoglob
shell option.gash- Not targeted for the time being due to missing crucial feature:command -p
.mrsh- Not targeted for the time being due to many deficiencies, e.g. Function output cannot be redirected emersion/mrsh#196 and command builtin without arguments crashes emersion/mrsh#204.